MT-22401: Add email campaigns commands - #9
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new ChangesEmail campaigns
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant EmailCampaignCommands
participant APIClient
participant EmailCampaignAPI
participant Output
CLI->>EmailCampaignCommands: execute campaign command
EmailCampaignCommands->>APIClient: create authenticated client
APIClient->>EmailCampaignAPI: request campaign resource or action
EmailCampaignAPI-->>APIClient: campaign or statistics response
APIClient-->>EmailCampaignCommands: decoded response
EmailCampaignCommands->>Output: render configured output
Output-->>CLI: formatted result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/commands/emailcampaigns/list.go`:
- Line 121: Update the perPage flag binding in the campaign list command to use
50 as its default, matching the advertised help text, while preserving the
existing Changed check and surrounding behavior.
In `@internal/commands/emailcampaigns/start.go`:
- Around line 14-33: Update the API request flow in newLifecycleCmd and the
underlying internal/client.Client configuration so Email Campaigns calls cannot
block indefinitely. Replace the unbounded context.Background() request context
with an appropriate deadline or configure the HTTP client with a suitable
timeout, preserving the existing campaignPath and response handling.
In `@internal/commands/emailcampaigns/update.go`:
- Around line 29-32: Update the email-campaign update flow around
buildAttributesBody and the Patch call to validate that at least one mutable
attribute was provided; return a validation error before issuing Patch when the
body contains no changes. Add a regression test covering an update with only
--id and verify no request is sent.
In `@README.md`:
- Line 129: Update the README schedule command’s --datetime value to a future or
clearly placeholder timestamp, and ensure the corresponding Mailtrap app
examples include a matching schedule example.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b5236ce-3368-40c0-9482-c3adc59cede6
📒 Files selected for processing (16)
README.mdcmd/root.gointernal/commands/emailcampaigns/attributes.gointernal/commands/emailcampaigns/cancel.gointernal/commands/emailcampaigns/create.gointernal/commands/emailcampaigns/delete.gointernal/commands/emailcampaigns/emailcampaigns.gointernal/commands/emailcampaigns/emailcampaigns_test.gointernal/commands/emailcampaigns/get.gointernal/commands/emailcampaigns/list.gointernal/commands/emailcampaigns/reset.gointernal/commands/emailcampaigns/schedule.gointernal/commands/emailcampaigns/start.gointernal/commands/emailcampaigns/stats.gointernal/commands/emailcampaigns/terminate.gointernal/commands/emailcampaigns/update.go
| func newLifecycleCmd(f *cmdutil.Factory, action, short string) *cobra.Command { | ||
| var campaignID string | ||
|
|
||
| cmd := &cobra.Command{ | ||
| Use: action, | ||
| Short: short, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| if err := cmdutil.RequireFlag("id", campaignID); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| c, err := f.NewClient() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var resp campaignResponse | ||
| if err := c.Post(context.Background(), client.BaseGeneral, campaignPath(campaignID, action), nil, &resp); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether internal/client.Client sets a default request/dial timeout.
ast-grep outline internal/client --items all
rg -n -A5 -B5 'Timeout' internal/clientRepository: mailtrap/mailtrap-cli
Length of output: 1158
🏁 Script executed:
#!/bin/bash
set -u
printf '--- client.go ---\n'
cat -n internal/client/client.go
printf '\n--- relevant command timeout context usages ---\n'
rg -n 'context\.Background|context\.With|Timeout|Deadline|ctx' internal/client internal/commands/emailcampaignsRepository: mailtrap/mailtrap-cli
Length of output: 10909
🌐 Web query:
Go net/http New default http.Client Timeout documentation
💡 Result:
The Go net/http package does not impose a default timeout on the standard http.Client [1][2]. The http.DefaultClient is initialized with a zero value for its Timeout field, which explicitly means "no timeout" [1][3]. In the Go documentation, the Timeout field of the http.Client struct is defined as follows [1][4]: Timeout specifies a time limit for requests made by this Client. The timeout includes connection time, any redirects, and reading the response body. A Timeout of zero means no timeout. Because http.DefaultClient has no timeout, using it or the convenience functions that rely on it (such as http.Get, http.Post, and http.Head) is generally discouraged for production applications, especially those interacting with the public internet, as it can lead to resource leaks and hung requests [5][6]. Developers are encouraged to define a custom http.Client with a specified Timeout or to use context.Context with requests to enforce deadlines [7][5].
Citations:
- 1: https://pkg.go.dev/net/http
- 2: https://go.dev/src/net/http/client.go?m=text
- 3: https://go.googlesource.com/go/+/master/src/net/http/client.go
- 4: https://tip.golang.org/pkg/http
- 5: https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/
- 6: https://petr-korobeinikov.com/posts/go-http-defaultclient/
- 7: https://stackoverflow.com/questions/16895294/how-to-set-timeout-for-http-get-requests-in-golang
Add timeout coverage for context.Background() API calls.
internal/client.Client creates &http.Client{} with no Timeout, and these commands pass context.Background() by value, so Email Campaigns requests can block indefinitely if the API response hangs. Add a request deadline or configure the HTTP client with an appropriate timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/commands/emailcampaigns/start.go` around lines 14 - 33, Update the
API request flow in newLifecycleCmd and the underlying internal/client.Client
configuration so Email Campaigns calls cannot block indefinitely. Replace the
unbounded context.Background() request context with an appropriate deadline or
configure the HTTP client with a suitable timeout, preserving the existing
campaignPath and response handling.
There was a problem hiding this comment.
The shared internal/client.Client predates this PR and every existing command has the same behavior, so a timeout belongs in a dedicated change to the shared client rather than this campaigns PR. Leaving this thread open as a note for that follow-up.
There was a problem hiding this comment.
@Rabsztok, agreed. The timeout concern applies to the shared internal/client.Client, not specifically to Email Campaigns. It should be evaluated in a dedicated change that defines the CLI-wide timeout behavior.
Would you like me to create a follow-up GitHub issue for this work?
✏️ Learnings added
Learnt from: Rabsztok
Repo: mailtrap/mailtrap-cli PR: 9
File: internal/commands/emailcampaigns/start.go:14-33
Timestamp: 2026-08-07T10:00:38.817Z
Learning: In the mailtrap/mailtrap-cli repository, request timeout behavior is shared by `internal/client.Client` and applies to all existing commands. Do not require a command-specific timeout change in a feature PR such as `internal/commands/emailcampaigns`; track shared client timeout changes in a dedicated follow-up.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
|
Addressed CodeRabbit findings in 06ed4f5: Fixed
Skipped
|
|
Follow-up from a cross-SDK contract audit: the API treats audience ids as the full set (explicit |
507c382 to
13e6844
Compare
Decisions: - Request bodies are flat (no email_campaign wrapper); nested reply_to/template_attributes are assembled from flat flags - Single, list, and stats responses unwrap the data envelope; list pagination metadata is ignored for output - Endpoint is token-scoped (/api/email_campaigns) — no account-id requirement, unlike other resources - Lifecycle actions (start/schedule/cancel/terminate/reset) are subcommands sharing one body-less POST helper - Delete handles 204 No Content with a confirmation message per repo convention
13e6844 to
5ccc323
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/commands/emailcampaigns/attributes.go`:
- Around line 52-53: The campaign sending-domain flow currently uses numeric
domain_id instead of the OpenAPI mailsend_domain_id UUID string. Update the
--domain-id flag and campaignAttrs in
internal/commands/emailcampaigns/attributes.go, serialize the changed flag as
mailsend_domain_id, and update the fixtures and assertions in
internal/commands/emailcampaigns/emailcampaigns_test.go at lines 47-51 and
218-223 to use UUID strings and the new request key.
In `@internal/commands/emailcampaigns/stats.go`:
- Around line 13-47: Add message_count and reject_count fields to CampaignStats
with matching JSON tags, then include both fields in campaignStatsColumns so the
decoded API totals are exposed in table output.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f06249b3-2fbf-4e22-81a5-80e1e2a810a0
📒 Files selected for processing (16)
README.mdcmd/root.gointernal/commands/emailcampaigns/attributes.gointernal/commands/emailcampaigns/cancel.gointernal/commands/emailcampaigns/create.gointernal/commands/emailcampaigns/delete.gointernal/commands/emailcampaigns/emailcampaigns.gointernal/commands/emailcampaigns/emailcampaigns_test.gointernal/commands/emailcampaigns/get.gointernal/commands/emailcampaigns/list.gointernal/commands/emailcampaigns/reset.gointernal/commands/emailcampaigns/schedule.gointernal/commands/emailcampaigns/start.gointernal/commands/emailcampaigns/stats.gointernal/commands/emailcampaigns/terminate.gointernal/commands/emailcampaigns/update.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/commands/emailcampaigns/delete.go
- internal/commands/emailcampaigns/reset.go
- internal/commands/emailcampaigns/cancel.go
- README.md
- internal/commands/emailcampaigns/get.go
- internal/commands/emailcampaigns/start.go
- internal/commands/emailcampaigns/emailcampaigns.go
- cmd/root.go
- internal/commands/emailcampaigns/schedule.go
- internal/commands/emailcampaigns/create.go
- internal/commands/emailcampaigns/terminate.go
- internal/commands/emailcampaigns/list.go
Decisions: - Bind --per-page default to 50 (matches API default per OpenAPI spec); Changed check kept so the param is only sent when set explicitly - README schedule example now uses an obviously-future timestamp (2030-01-01) - No timeout added for context.Background() calls: matches repo convention (no command uses timeouts); a CLI-wide timeout belongs in a separate change - update with no attribute flags now fails fast with a validation error instead of sending an empty PATCH
Decisions: - The API treats contact_list_ids/contact_segment_ids as the full set, so an explicit [] clears them — pflag cannot parse an empty slice value, so dedicated --clear-contact-lists/--clear-contact-segments booleans express it; mutually exclusive with the ids flags
5ccc323 to
d1ec54c
Compare
1 participant
Motivation
MT-22401
Port the Email Campaigns public API (MT-21113) to the CLI.
Changes
mailtrap email-campaignswith 12 subcommands:list(--per-page/--search/--token),get,create,update(PATCH semantics — only changed flags are sent),delete, the five lifecycle actions (start,schedule --datetime,cancel,terminate,reset), andstats(--start-date/--end-date)data-envelope unwrapping, integer--domain-id(matching the ids returned by the Sending Domains endpoints), nestedreply_to/template_attributes/delivery_optionsbuilt from flat flags, audience id slicesHow to test
mailtrap email-campaigns create --name "Test" --domain-id <id> --from-local-part news --subject "Hi"with a real token — expect a draft campaign rowschedule --datetime <future ISO>→cancel→start→terminate; verify state transitions and that invalid transitions print the API erroremail-campaigns list --search <name>andstats <id> --start-date ... --end-date ...return sensible tables and--output jsonparsesSummary by CodeRabbit